Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add Support for Customising the Shell for Pre and Post Workflow Hooks #3451

Merged
merged 13 commits into from
Jul 7, 2023
Merged

feat: Add Support for Customising the Shell for Pre and Post Workflow Hooks #3451

merged 13 commits into from
Jul 7, 2023

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented May 25, 2023 •
edited
Loading

what

Update the pre and post workflow hooks with additional shell and shellArgs properties to allow customisation of the shell
used to run the hooks.

Grammar fixes have also been actioned on the contents of the runatlantis.io/docs/pre-workflow-hooks.md file.

why

tests

Default Shell

repos:
- id: /.*/
  pre_workflow_hooks:
    -  description: pre workflow shell test
       run: echo "pre workflow $0"
  post_workflow_hooks:
    -  description: post workflow shell test
       run: echo "post workflow $0"

Pre Workflow Output

image

Post Workflow Output

image

Custom Shell

repos.yaml

repos:
- id: /.*/
  pre_workflow_hooks:
    -  description: pre workflow shell test
       shell: bash
       run: echo "pre workflow $0"
  post_workflow_hooks:
    -  description: post workflow shell test
       shell: bash
       run: echo "post workflow $0"

Pre Workflow Output

image

Post Workflow Output

image

Custom ShellArgs

repos.yaml

repos:
- id: /.*/
  pre_workflow_hooks:
    -  description: pre workflow shell test
       shellArgs: -cx
       run: echo "pre workflow $0"
  post_workflow_hooks:
    -  description: post workflow shell test
       shellArgs: -cx
       run: echo "post workflow $0"

Pre Workflow Output

image

Post Workflow Output

image

Custom Shell and ShellArgs

repos.yaml

repos:
- id: /.*/
  pre_workflow_hooks:
    -  description: pre workflow shell test
       shell: bash
       shellArgs: -cv
       run: echo "pre workflow $0"
  post_workflow_hooks:
    -  description: post workflow shell test
       shell: bash
       shellArgs: -cv
       run: echo "post workflow $0"

Pre Workflow Output

image

Post Workflow Output

image

@X-Guardian X-Guardian requested a review from a team as a code owner May 25, 2023 15:15
@github-actions github-actions bot added docs Documentation go Pull requests that update Go code labels May 25, 2023
@nitrocode
Copy link
Member

Thank you for the contribution but can we also add this for run steps, not just for pre and post hooks?

@X-Guardian
Copy link
Contributor Author

I'm sure that is possible, but is outside the scope of this issue/feature request, so not within this PR.

@nitrocode
Copy link
Member

To me, it doesn't make sense to do it in one section and not in another. It's confusing and some people will reach out and ask why it's implemented in workflow hooks which is a recent feature but not in "steps" which is the one of the oldest features.

Please reconsider.

@X-Guardian
Copy link
Contributor Author

I hate scope creep in PRs. The code is completely separate for the two features, so it can be done in a different PR.

@jamengual
Copy link
Contributor

I will prefer in different PRs, pre and post workflow hooks are very specific steps compared with run and some people do not use pre/post workflows but I will say 99% people use the run command. Separating the PR make it easier to revert in case affects any of the two.

@nitrocode
Copy link
Member

I appreciate your work and am thankful for this contribution. I'm just concerned that this pr may merge and a follow up for overriding the shell for run steps isn't contributed leaving the project with a half finished feature. I'm ok with 2 PRs as well.

@nitrocode
Copy link
Member

One thing I'd like to call out too. It's odd that this logic is in multiple places which then requires separate prs. We'd want the pre/post/regular run steps to all use the same logic so if the logic was updated, it would impact all 3. It would seem that all 3 are currently using a copy and pasted interface instead of using a shared interface.

It would be a great contribution if the follow up PR could help refactor and make the code more DRY. But i know i may be asking for too much here. Thank you again for your effort.

@X-Guardian X-Guardian requested a review from nitrocode May 31, 2023 17:08
@X-Guardian
Copy link
Contributor Author

@nitrocode, can you re-review this PR?

@nitrocode
Copy link
Member

Hi @X-Guardian , i appreciate all your work but my original comment hasn't been addressed.

I'd prefer to merge a pr to change the interpretter for the individual run steps prior to merging this pr for pre and post workflow hooks.

@X-Guardian
Copy link
Contributor Author

OK @nitrocode. I have to say I am disappointed with your response to this. Just so you know, I won't be implementing this request as I have no need for it, and the original scope of the issue and PR was always only for the pre/post workflows.

@nitrocode
Copy link
Member

I'm also disappointed as I think if and when this pr is merged, the implementation will be half baked because it will be implemented on workflow hooks but not run steps. I also understand that you do not want to finish that implementation. Please remember that this is volunteer based on both contributors and reviewers.

cc @jamengual has volunteered to take over this. Thanks for being patient @X-Guardian

jamengual
jamengual previously approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jamengual
Copy link
Contributor

@GenPage any thoughs?

GenPage
GenPage reviewed Jun 15, 2023
View reviewed changes
Copy link
Member

@GenPage GenPage left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can resolve this easily without disappointments. It would be an easy quick win to add this feature to the run_step_runner based on the changes I see here.

However, it does not seem prudent to gate this PR on the basis of requiring the contributor to perform a refactor or expanding PR scope if they've already refused. This project is very much heavily driven by contributors in terms of adding features and code changes.

Given the situation, if someone wants this added to the run step, they will have a solid template for that work in this PR.

GenPage
GenPage previously approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

server/events/post_workflow_hooks_command_runner.go Outdated Show resolved Hide resolved
server/events/post_workflow_hooks_command_runner.go Outdated Show resolved Hide resolved
server/events/pre_workflow_hooks_command_runner.go Outdated Show resolved Hide resolved
server/events/pre_workflow_hooks_command_runner.go Outdated Show resolved Hide resolved
@X-Guardian X-Guardian dismissed stale reviews from GenPage and jamengual via e749fd5 June 16, 2023 08:22
@X-Guardian X-Guardian requested a review from GenPage June 16, 2023 08:31
jamengual
jamengual previously approved these changes Jun 19, 2023
View reviewed changes
@X-Guardian
Copy link
Contributor Author

@GenPage, @jamengual this PR needs another review as I needed to resolve some conflicts.

@X-Guardian X-Guardian requested a review from jamengual July 2, 2023 11:47
GenPage
GenPage previously approved these changes Jul 7, 2023
View reviewed changes
@GenPage GenPage enabled auto-merge (squash) July 7, 2023 20:23
@GenPage
Copy link
Member

GenPage commented Jul 7, 2023

@X-Guardian Looks like the other PR conflicts the tests with this one.

auto-merge was automatically disabled July 7, 2023 21:26

Head branch was pushed to by a user without write access

@GenPage GenPage merged commit b899419 into runatlantis:main Jul 7, 2023
@X-Guardian X-Guardian deleted the shell branch July 7, 2023 22:34
@GenPage GenPage mentioned this pull request Aug 12, 2023
Closed
@ghaiszaher ghaiszaher mentioned this pull request Aug 16, 2023
Merged
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
@X-Guardian @ijames-gc
feat: Add Support for Customising the Shell for Pre and Post Workflow…
f55088a 
… Hooks (runatlantis#3451)
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
@X-Guardian @ijames-gc
feat: Add Support for Customising the Shell for Pre and Post Workflow…
736501f 
… Hooks (runatlantis#3451)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GenPage GenPage GenPage approved these changes

@nitrocode nitrocode Awaiting requested review from nitrocode

@jamengual jamengual Awaiting requested review from jamengual

Assignees
No one assigned
Labels
docs Documentation go Pull requests that update Go code
Projects
None yet
Milestone
No milestone
4 participants
@X-Guardian @nitrocode @jamengual @GenPage